Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump to GHC 8.10.4 #2533

Merged
merged 36 commits into from
Mar 17, 2021
Merged

Bump to GHC 8.10.4 #2533

merged 36 commits into from
Mar 17, 2021

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Feb 22, 2021

Issue Number

ADP-453

Overview

  • Bump to GHC-8.10.4
  • Use OnDeleteCascade in new persistent version, instead of our workaround in the fork
Todo:
  • Remove allow-newer
  • core:unit tests pass locally
  • Look closer at what why we needed the persistent fork
    • Seems we can now simply use OnDeleteCascade
  • Integration tests pass locally
  • Investigate cardano-wallet:unit failures
  • Bump haskell.nix
  • Figure out how to use weeder -> seems green at least
  • Get CI green
  • Move to snapshot in cardano-haskell repo
  • Merge that first

Comments

Hydra jobset

@Anviking Anviking self-assigned this Feb 22, 2021
@Anviking Anviking closed this Mar 6, 2021
@Anviking Anviking reopened this Mar 10, 2021
@Anviking Anviking force-pushed the anviking/ADP-453/bump-ghc branch from cf4d2b7 to 8937133 Compare March 10, 2021 14:59
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good so far, 🎉.
We need to double check auto db migrations still work.
I'll work on the haskell.nix bump.

@@ -85,7 +84,7 @@ PrivateKey sql=private_key
privateKeyHash B8.ByteString sql=hash

Primary privateKeyWalletId
Foreign Wallet fk_wallet_private_key privateKeyWalletId ! ON DELETE CASCADE
Foreign Wallet OnDeleteCascade fk_wallet_private_key privateKeyWalletId
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be able to make a fresh database and inspect the output of .schema private_key?
There should be an ON DELETE CASCADE in the wallet fk.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sqlite3 wallet-db/she.908bd12c214e1883b6ba1ca2cb5d7066a6a2cc30.sqlite
SQLite version 3.33.0 2020-08-14 13:23:32
Enter ".help" for usage hints.
sqlite>
sqlite> .schema private_key
CREATE TABLE IF NOT EXISTS "private_key"("wallet_id" VARCHAR NOT NULL,"root" BLOB NOT NULL,"hash" BLOB NOT NULL, PRIMARY KEY ("wallet_id"), CONSTRAINT "private_keyfk_wallet_private_key" FOREIGN KEY("wallet_id") REFERENCES "wallet"("wallet_id") ON DELETE CASCADE);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be the same as on an old DB:

sqlite3 wallet-db4/she.0e016aeb9828d1f61d5192b6e1bdea69f3d52017.sqlite
SQLite version 3.33.0 2020-08-14 13:23:32
Enter ".help" for usage hints.
sqlite> .schema private_key
CREATE TABLE IF NOT EXISTS "private_key"("wallet_id" VARCHAR NOT NULL,"root" BLOB NOT NULL,"hash" BLOB NOT NULL, PRIMARY KEY ("wallet_id"), CONSTRAINT "private_keyfk_wallet_private_key" FOREIGN KEY("wallet_id") REFERENCES "wallet"("wallet_id") ON DELETE CASCADE);
sqlite>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to double check auto db migrations still work.

Starting the wallet on some old DB worked. And the unit tests failed when the new unsafe migrations weren't enabled.

That should be enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're talking about these unit tests:

https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/test/unit/Cardano/Wallet/DB/SqliteSpec.hs#L262-L406

Then I'd say that your arguments are pretty convincing yes.

lib/core/src/Cardano/Wallet/DB/Sqlite/Types.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/Api/Server/TlsSpec.hs Outdated Show resolved Hide resolved
stack.yaml Outdated Show resolved Hide resolved
@Anviking Anviking force-pushed the anviking/ADP-453/bump-ghc branch from 0eba54e to 05c346e Compare March 11, 2021 10:33
@Anviking Anviking changed the title wip: Bump to GHC 8.10.4 Bump to GHC 8.10.4 Mar 11, 2021
@Anviking Anviking force-pushed the anviking/ADP-453/bump-ghc branch from 938f514 to 8a182a2 Compare March 11, 2021 14:19
@rvl rvl force-pushed the anviking/ADP-453/bump-ghc branch 4 times, most recently from eac72b8 to 5c87c77 Compare March 12, 2021 05:01
@rvl
Copy link
Contributor

rvl commented Mar 12, 2021

Almost there.
There are a few interesting unit test failures.
The windows nix builds are failing on the regex-posix dependency.

@Anviking Anviking force-pushed the anviking/ADP-453/bump-ghc branch from 980a66f to c1a5f9d Compare March 12, 2021 15:44
@Anviking
Copy link
Member Author

Hm, yes, these are interesting 😮

  test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs:436:17:
  1) Cardano.Wallet.Shelley.Transaction, estimateMaxNumberOfInputs for ShelleyKey, order of magnitude, nOuts = 1 => nInps = 114
       expected: 114
        but got: 115

  To rerun use: --match "/Cardano.Wallet.Shelley.Transaction/estimateMaxNumberOfInputs for ShelleyKey/order of magnitude, nOuts = 1 => nInps = 114/"

  test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs:436:17:
  2) Cardano.Wallet.Shelley.Transaction, estimateMaxNumberOfInputs for ShelleyKey, order of magnitude, nOuts = 5 => nInps = 104
       expected: 104
        but got: 106

  To rerun use: --match "/Cardano.Wallet.Shelley.Transaction/estimateMaxNumberOfInputs for ShelleyKey/order of magnitude, nOuts = 5 => nInps = 104/"

  test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs:436:17:
  3) Cardano.Wallet.Shelley.Transaction, estimateMaxNumberOfInputs for ShelleyKey, order of magnitude, nOuts = 10 => nInps = 99
       expected: 99
        but got: 101

  To rerun use: --match "/Cardano.Wallet.Shelley.Transaction/estimateMaxNumberOfInputs for ShelleyKey/order of magnitude, nOuts = 10 => nInps = 99/"

  test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs:436:17:
  4) Cardano.Wallet.Shelley.Transaction, estimateMaxNumberOfInputs for ShelleyKey, order of magnitude, nOuts = 20 => nInps = 75
       expected: 75
        but got: 85

  To rerun use: --match "/Cardano.Wallet.Shelley.Transaction/estimateMaxNumberOfInputs for ShelleyKey/order of magnitude, nOuts = 20 => nInps = 75/"

  test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs:436:17:
  5) Cardano.Wallet.Shelley.Transaction, estimateMaxNumberOfInputs for ShelleyKey, order of magnitude, nOuts = 50 => nInps = 34
       expected: 34
        but got: 32

  To rerun use: --match "/Cardano.Wallet.Shelley.Transaction/estimateMaxNumberOfInputs for ShelleyKey/order of magnitude, nOuts = 50 => nInps = 34/"

  test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs:436:17:
  6) Cardano.Wallet.Shelley.Transaction, estimateMaxNumberOfInputs for ByronKey, order of magnitude, nOuts = 5 => nInps = 66
       expected: 66
        but got: 67

  To rerun use: --match "/Cardano.Wallet.Shelley.Transaction/estimateMaxNumberOfInputs for ByronKey/order of magnitude, nOuts = 5 => nInps = 66/"

  test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs:436:17:
  7) Cardano.Wallet.Shelley.Transaction, estimateMaxNumberOfInputs for ByronKey, order of magnitude, nOuts = 10 => nInps = 62
       expected: 62
        but got: 63

  To rerun use: --match "/Cardano.Wallet.Shelley.Transaction/estimateMaxNumberOfInputs for ByronKey/order of magnitude, nOuts = 10 => nInps = 62/"

  test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs:436:17:
  8) Cardano.Wallet.Shelley.Transaction, estimateMaxNumberOfInputs for ByronKey, order of magnitude, nOuts = 20 => nInps = 45
       expected: 45
        but got: 52

  To rerun use: --match "/Cardano.Wallet.Shelley.Transaction/estimateMaxNumberOfInputs for ByronKey/order of magnitude, nOuts = 20 => nInps = 45/"

  test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs:436:17:
  9) Cardano.Wallet.Shelley.Transaction, estimateMaxNumberOfInputs for ByronKey, order of magnitude, nOuts = 50 => nInps = 16
       expected: 16
        but got: 14

  To rerun use: --match "/Cardano.Wallet.Shelley.Transaction/estimateMaxNumberOfInputs for ByronKey/order of magnitude, nOuts = 50 => nInps = 16/"

  test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs:436:17:
  10) Cardano.Wallet.Shelley.Transaction, estimateMaxNumberOfInputs for IcarusKey, order of magnitude, nOuts = 5 => nInps = 66
       expected: 66
        but got: 67

  To rerun use: --match "/Cardano.Wallet.Shelley.Transaction/estimateMaxNumberOfInputs for IcarusKey/order of magnitude, nOuts = 5 => nInps = 66/"

  test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs:436:17:
  11) Cardano.Wallet.Shelley.Transaction, estimateMaxNumberOfInputs for IcarusKey, order of magnitude, nOuts = 10 => nInps = 62
       expected: 62
        but got: 63

  To rerun use: --match "/Cardano.Wallet.Shelley.Transaction/estimateMaxNumberOfInputs for IcarusKey/order of magnitude, nOuts = 10 => nInps = 62/"

  test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs:436:17:
  12) Cardano.Wallet.Shelley.Transaction, estimateMaxNumberOfInputs for IcarusKey, order of magnitude, nOuts = 20 => nInps = 45
       expected: 45
        but got: 52

  To rerun use: --match "/Cardano.Wallet.Shelley.Transaction/estimateMaxNumberOfInputs for IcarusKey/order of magnitude, nOuts = 20 => nInps = 45/"

  test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs:436:17:
  13) Cardano.Wallet.Shelley.Transaction, estimateMaxNumberOfInputs for IcarusKey, order of magnitude, nOuts = 50 => nInps = 16
       expected: 16
        but got: 14

  To rerun use: --match "/Cardano.Wallet.Shelley.Transaction/estimateMaxNumberOfInputs for IcarusKey/order of magnitude, nOuts = 50 => nInps = 16/"

Randomized with seed 1942845292

Finished in 18.3526 seconds
153 examples, 13 failures

I hadn't tried running this suite before...

@KtorZ
Copy link
Member

KtorZ commented Mar 16, 2021

#2533 (comment)

Interesting 🤔 ... could it be that some of the generators changed in newer revisions of QuickCheck 🤔 ?

@Anviking
Copy link
Member Author

Interesting 🤔 ... could it be that some of the generators changed in newer revisions of QuickCheck 🤔 ?

I assumed something like that is the case -> 870f971

@@ -222,7 +224,7 @@ chainSyncFollowTip toCardanoEra onTipUpdate =
-- callback.
--
-- See also 'send' for invoking commands.
data ChainSyncCmd block (m :: * -> *)
data ChainSyncCmd block (m :: Type -> Type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting to wonder if enabling {-# LANGUAGE StarIsType #-} as a default extension wouldn't have made your life easier here 😅 ... I guess going over this change is now for the better.

@@ -431,6 +431,9 @@ estimateMaxInputsTests cases = do
forM_ cases $ \(GivenNumOutputs nOuts, ExpectedNumInputs nInps) -> do
let (o,i) = (show nOuts, show nInps)
it ("order of magnitude, nOuts = " <> o <> " => nInps = " <> i) $ do
-- NOTE: These tests broke in the GHC 8.6 -> 8.10 bump,
-- presumably due to some change in the arbitrary generation.
-- It would be better if they weren't so fragile.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could perhaps "dump" the generated values in files used as input data for the test?

Copy link
Member Author

@Anviking Anviking Mar 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably - don't think it'd be worth the effort though. Would be 50 outputs in the source file.

But I wonder if this wouldn't be nice to some day have:

  • 50 arbitrary outputs -> at least X1 inputs, never more than X2 inputs
  • 50 fixed-size outputs -> always Y inputs

i.e. properties rather than goldens.

@Anviking
Copy link
Member Author

nix-shell nix/cabal-shell.nix --run "cabal v2-configure -frelease --enable-tests --enable-benchmarks"

seems to be failing (and some windows hydra jobs too)

Does @rvl have a clue?

- hedgehog-1.0.2
- hedgehog-corpus-0.2.0
- hedgehog-quickcheck-0.1.1
- hspec-2.7.0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to use a more recent hspec for the beforeAllWith function when trying a thing.

I think there's no point in having this pinned to an old version.

Maybe we shouldn't drop non-test-related pinned packages though, if we want them to perfectly match what's used in cardano-node.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we may as well use the hspec-2.7.8 version from the LTS snapshot.

If we change hedgehog versions, that may possibly break the build. We may as well leave it if it works.

cabal.project Outdated
Comment on lines 173 to 199
-- The following is needed because Nix is doing something crazy.
package byron-spec-ledger
tests: False

package ouroboros-consensus-test
tests: False

package ouroboros-consensus-cardano-test
tests: False

package ouroboros-network
tests: False

package ouroboros-network-framework
tests: False

package small-steps
tests: False

package small-steps-test
tests: False

package goblins
tests: False

package io-sim-classes
tests: False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block probably isn't required here.

- hedgehog-1.0.2
- hedgehog-corpus-0.2.0
- hedgehog-quickcheck-0.1.1
- hspec-2.7.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we may as well use the hspec-2.7.8 version from the LTS snapshot.

If we change hedgehog versions, that may possibly break the build. We may as well leave it if it works.

stack.yaml Outdated
# released cardano-node version, and supporting libraries.
#
# NOTE: Remember to update the version matrix in README.md when
# bumping the Cardano version.
resolver: https://raw.githubusercontent.com/input-output-hk/cardano-haskell/1c257e85cadb503ced0fe0a9ba757ae449baaf0b/snapshots/cardano-1.25.1.yaml
resolver: cardano-1.25.1.yaml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it time to add the snapshot to cardano-haskell?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Want to see if CI's green after un-pinning hspec, will move after that

cabal.project Outdated
tests: False
benchmarks: False
package iohk-monitoring
-- The following is needed because Nix is doing something crazy.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block of package options probably isn't necessary here.

Copy link
Member Author

@Anviking Anviking Mar 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was. It fixed the error from nix-shell nix/cabal-shell.nix --run "cabal v2-configure -frelease --enable-tests --enable-benchmarks". The first section wasn't needed though... but seems fine to me to keep.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like cabal ignores the global tests: False flag, and then disabling tests on a selection of difficult packages is enough to make it work. This probjem is not likely to be related to nix - because we are running cabal outside of nix-build.

@rvl rvl force-pushed the anviking/ADP-453/bump-ghc branch from 31b13f4 to b0d74e2 Compare March 17, 2021 07:34
@Anviking Anviking marked this pull request as ready for review March 17, 2021 09:52
@Anviking
Copy link
Member Author

bors r+

iohk-bors bot added a commit that referenced this pull request Mar 17, 2021
2533: Bump to GHC 8.10.4 r=Anviking a=Anviking

# Issue Number

ADP-453

# Overview

- [x] Bump to GHC-8.10.4
- [x] Use OnDeleteCascade in new persistent version, instead of our workaround in the fork

##### Todo:

- [x] Remove `allow-newer`
- [x] core:unit tests pass locally
- [x] Look closer at what why we needed the persistent fork
   - Seems we can now simply use OnDeleteCascade
- [x] Integration tests pass locally
- [x] Investigate cardano-wallet:unit failures
- [x] Bump haskell.nix
- [x] Figure out how to use weeder -> seems green at least
- [x] Get CI green
- [x] Move to snapshot in cardano-haskell repo
- [x] Merge that first

# Comments

[Hydra jobset](https://hydra.iohk.io/jobset/Cardano/cardano-wallet-pr-2533)


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 17, 2021

Build failed:

#expected

@Anviking Anviking force-pushed the anviking/ADP-453/bump-ghc branch from e356927 to e71c1ce Compare March 17, 2021 11:04
@Anviking
Copy link
Member Author

bors r+

iohk-bors bot added a commit that referenced this pull request Mar 17, 2021
2533: Bump to GHC 8.10.4 r=Anviking a=Anviking

# Issue Number

ADP-453

# Overview

- [x] Bump to GHC-8.10.4
- [x] Use OnDeleteCascade in new persistent version, instead of our workaround in the fork

##### Todo:

- [x] Remove `allow-newer`
- [x] core:unit tests pass locally
- [x] Look closer at what why we needed the persistent fork
   - Seems we can now simply use OnDeleteCascade
- [x] Integration tests pass locally
- [x] Investigate cardano-wallet:unit failures
- [x] Bump haskell.nix
- [x] Figure out how to use weeder -> seems green at least
- [x] Get CI green
- [x] Move to snapshot in cardano-haskell repo
- [x] Merge that first

# Comments

[Hydra jobset](https://hydra.iohk.io/jobset/Cardano/cardano-wallet-pr-2533)


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 17, 2021

Build failed:

cardano-wallet             >   src/Test/Integration/Scenario/CLI/Shelley/Transactions.hs:387:59:
--
  | cardano-wallet             >   1) CLI Specifications, SHELLEY_CLI_TRANSACTIONS, TRANS_ESTIMATE_09 - Invalid amount, -1000
  | cardano-wallet             >        uncaught exception: IOException of type ResourceVanished
  | cardano-wallet             >        fd:195: hFlush: resource vanished (Broken pipe)
  | cardano-wallet             >
  | cardano-wallet             >   To rerun use: --match "/CLI Specifications/SHELLEY_CLI_TRANSACTIONS/TRANS_ESTIMATE_09 - Invalid amount/-1000/"
  | cardano-wallet             >
  | cardano-wallet             > Randomized with seed 1166162994

@Anviking
Copy link
Member Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 17, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit f4859bf into master Mar 17, 2021
@iohk-bors iohk-bors bot deleted the anviking/ADP-453/bump-ghc branch March 17, 2021 16:01
rvl added a commit that referenced this pull request Mar 26, 2021
The meaning of $src seems to have changed since the Haskell.nix bump
in PR #2533.
@rvl rvl mentioned this pull request Mar 26, 2021
piotr-iohk pushed a commit that referenced this pull request Mar 26, 2021
The meaning of $src seems to have changed since the Haskell.nix bump
in PR #2533.
iohk-bors bot added a commit that referenced this pull request Mar 27, 2021
2576: Fix nightly benchmarks r=rvl a=rvl

### Issue Number

ADP-804

### Overview

Nightly restore bench is still failing.

1. Restore bench: Increase cardano-node startup time allowance

   If the cardano-node database is shared between bench runs, it may need to rebuild the ledger, etc, at startup. So increase the time available for that.

2. Latency bench: Fix working directory

   The meaning of `$src` seems to have changed since the Haskell.nix bump in PR #2533.

3. DB Bench: I broke the criterion env setup - fixed now.



Co-authored-by: Rodney Lorrimar <[email protected]>
iohk-bors bot added a commit that referenced this pull request Mar 28, 2021
2576: Fix nightly benchmarks r=Anviking a=rvl

### Issue Number

ADP-804

### Overview

Nightly restore bench is still failing.

1. Restore bench: Increase cardano-node startup time allowance

   If the cardano-node database is shared between bench runs, it may need to rebuild the ledger, etc, at startup. So increase the time available for that.

2. Latency bench: Fix working directory

   The meaning of `$src` seems to have changed since the Haskell.nix bump in PR #2533.

3. DB Bench: I broke the criterion env setup - fixed now.



Co-authored-by: Rodney Lorrimar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants